Self-Improvement framework#243
Conversation
There was a problem hiding this comment.
Code Review
This pull request restructures the codebase by removing the old chunker and condenser implementations and introducing a new progressive distillation framework. This includes a revised base Condenser that utilizes a teacher-student routing decorator, a new FactsCondenser, a Classifier base class, training utilities, and the llm_backup utility. Feedback on these changes highlights several critical issues: first, Condenser._sample may raise an IndexError if the sampler returns a generator, which can be fixed by converting responses to a list first; second, llm_backup_async blocks the asyncio event loop by calling a synchronous network-bound function, which should be wrapped in asyncio.to_thread; and third, the argument parsing in llm_backup is fragile and should be refactored to use inspect.signature's bind and apply_defaults for robust argument binding.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| responses = self.sampler.sample([trajectory], **sample_kwargs) | ||
| return self._decoded(list(responses)[0]) if responses else '' |
There was a problem hiding this comment.
If self.sampler.sample returns a generator or iterator (which is common for streaming or lazy samplers), the check if responses will evaluate to True because generator/iterator objects are always truthy in Python, even if they yield no elements. Subsequently, calling list(responses)[0] on an empty generator will raise an IndexError: list index out of range.
To prevent this, convert responses to a list first, and then check if the list is non-empty before accessing the first element.
| responses = self.sampler.sample([trajectory], **sample_kwargs) | |
| return self._decoded(list(responses)[0]) if responses else '' | |
| responses = list(self.sampler.sample([trajectory], **sample_kwargs)) | |
| return self._decoded(responses[0]) if responses else '' |
| if use_student: | ||
| result = await fn(*args, **kwargs) | ||
| if random.random() < sample_rate: | ||
| teacher_result = _call_teacher(trajectory, sampling_params) | ||
| match = comparator(result, teacher_result) | ||
| _registry.add_record(key, result, teacher_result, match) | ||
| if not match: | ||
| result = teacher_result | ||
| else: | ||
| teacher_result = _call_teacher(trajectory, sampling_params) | ||
| student_result = await fn(*args, **kwargs) | ||
| match = comparator(student_result, teacher_result) | ||
| _registry.add_record(key, student_result, teacher_result, match) | ||
| result = teacher_result | ||
|
|
There was a problem hiding this comment.
In llm_backup_async, calling the synchronous network-bound function _call_teacher directly will block the entire asyncio event loop for the duration of the API call (which can take several seconds). This defeats the purpose of using asynchronous programming.
Use asyncio.to_thread to run the synchronous _call_teacher function in a separate thread, preventing it from blocking the event loop.
| if use_student: | |
| result = await fn(*args, **kwargs) | |
| if random.random() < sample_rate: | |
| teacher_result = _call_teacher(trajectory, sampling_params) | |
| match = comparator(result, teacher_result) | |
| _registry.add_record(key, result, teacher_result, match) | |
| if not match: | |
| result = teacher_result | |
| else: | |
| teacher_result = _call_teacher(trajectory, sampling_params) | |
| student_result = await fn(*args, **kwargs) | |
| match = comparator(student_result, teacher_result) | |
| _registry.add_record(key, student_result, teacher_result, match) | |
| result = teacher_result | |
| if use_student: | |
| result = await fn(*args, **kwargs) | |
| if random.random() < sample_rate: | |
| import asyncio | |
| teacher_result = await asyncio.to_thread(_call_teacher, trajectory, sampling_params) | |
| match = comparator(result, teacher_result) | |
| _registry.add_record(key, result, teacher_result, match) | |
| if not match: | |
| result = teacher_result | |
| else: | |
| import asyncio | |
| teacher_result = await asyncio.to_thread(_call_teacher, trajectory, sampling_params) | |
| student_result = await fn(*args, **kwargs) | |
| match = comparator(student_result, teacher_result) | |
| _registry.add_record(key, student_result, teacher_result, match) | |
| result = teacher_result |
| def _build_key(func_name: str, args: tuple, kwargs: dict, | ||
| param_names: List[str], key_params: Sequence[str]) -> str: | ||
| """Build a unique key from specified parameter values.""" | ||
| key_parts = [func_name] | ||
| for i, name in enumerate(param_names): | ||
| if name in key_params: | ||
| if i < len(args): | ||
| key_parts.append(f"{name}={_serialize_value(args[i])}") | ||
| elif name in kwargs: | ||
| key_parts.append(f"{name}={_serialize_value(kwargs[name])}") | ||
| for name in key_params: | ||
| if name not in param_names[:len(args)] and name in kwargs: | ||
| if f"{name}={_serialize_value(kwargs[name])}" not in key_parts: | ||
| key_parts.append(f"{name}={_serialize_value(kwargs[name])}") | ||
| raw_key = "|".join(key_parts) | ||
| return hashlib.md5(raw_key.encode()).hexdigest() | ||
|
|
||
|
|
||
| def _serialize_value(value: Any) -> str: | ||
| try: | ||
| return json.dumps(value, sort_keys=True, default=str) | ||
| except (TypeError, ValueError): | ||
| return str(value) | ||
|
|
||
|
|
||
| def _extract_param(args: tuple, kwargs: dict, param_names: List[str], name: str) -> Any: | ||
| """Extract a named parameter from args/kwargs given the signature's param_names.""" | ||
| if name in kwargs: | ||
| return kwargs[name] | ||
| for i, pname in enumerate(param_names): | ||
| if pname == name and i < len(args): | ||
| return args[i] | ||
| return None |
There was a problem hiding this comment.
The current implementation of _build_key and _extract_param manually matches arguments by index and checks kwargs. This approach is fragile and does not handle default values correctly when parameters are omitted from the call (relying on their defaults). For example, if a parameter in key_params has a default value (like query=None) and is omitted from the call, it won't be included in the key, leading to different keys for semantically identical calls.
Using inspect.signature's bind and apply_defaults provides a robust, standard way to handle positional/keyword arguments and default values.
def _build_key(sig: inspect.Signature, func_name: str, args: tuple, kwargs: dict, key_params: Sequence[str]) -> str:
"""Build a unique key from specified parameter values using signature binding."""
try:
bound = sig.bind(*args, **kwargs)
bound.apply_defaults()
arguments = bound.arguments
except TypeError:
arguments = kwargs
key_parts = [func_name]
for name in key_params:
if name in arguments:
key_parts.append(f"{name}={_serialize_value(arguments[name])}")
raw_key = "|".join(key_parts)
return hashlib.md5(raw_key.encode()).hexdigest()
def _serialize_value(value: Any) -> str:
try:
return json.dumps(value, sort_keys=True, default=str)
except (TypeError, ValueError):
return str(value)
def _extract_param(sig: inspect.Signature, args: tuple, kwargs: dict, name: str) -> Any:
"""Extract a named parameter from args/kwargs given the signature."""
try:
bound = sig.bind(*args, **kwargs)
bound.apply_defaults()
return bound.arguments.get(name)
except TypeError:
return kwargs.get(name)| def wrapper(*args, **kwargs): | ||
| key = _build_key(fn.__qualname__, args, kwargs, param_names, key_params) | ||
| confidence = _registry.get_confidence(key) | ||
|
|
||
| try: | ||
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | ||
| except (ValueError, TypeError): | ||
| refresh_interval = default_refresh_interval | ||
|
|
||
| # Extract trajectory and sampling_params for teacher call | ||
| trajectory = _extract_param(args, kwargs, param_names, 'trajectory') | ||
| sampling_params = _extract_param(args, kwargs, param_names, 'sampling_params') |
There was a problem hiding this comment.
Update the call sites in llm_backup to pass the pre-computed sig object to _build_key and _extract_param for robust signature-based argument binding.
| def wrapper(*args, **kwargs): | |
| key = _build_key(fn.__qualname__, args, kwargs, param_names, key_params) | |
| confidence = _registry.get_confidence(key) | |
| try: | |
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | |
| except (ValueError, TypeError): | |
| refresh_interval = default_refresh_interval | |
| # Extract trajectory and sampling_params for teacher call | |
| trajectory = _extract_param(args, kwargs, param_names, 'trajectory') | |
| sampling_params = _extract_param(args, kwargs, param_names, 'sampling_params') | |
| def wrapper(*args, **kwargs): | |
| key = _build_key(sig, fn.__qualname__, args, kwargs, key_params) | |
| confidence = _registry.get_confidence(key) | |
| try: | |
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | |
| except (ValueError, TypeError): | |
| refresh_interval = default_refresh_interval | |
| # Extract trajectory and sampling_params for teacher call | |
| trajectory = _extract_param(sig, args, kwargs, 'trajectory') | |
| sampling_params = _extract_param(sig, args, kwargs, 'sampling_params') |
| async def wrapper(*args, **kwargs): | ||
| key = _build_key(fn.__qualname__, args, kwargs, param_names, key_params) | ||
| confidence = _registry.get_confidence(key) | ||
|
|
||
| try: | ||
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | ||
| except (ValueError, TypeError): | ||
| refresh_interval = default_refresh_interval | ||
|
|
||
| trajectory = _extract_param(args, kwargs, param_names, 'trajectory') | ||
| sampling_params = _extract_param(args, kwargs, param_names, 'sampling_params') |
There was a problem hiding this comment.
Update the call sites in llm_backup_async to pass the pre-computed sig object to _build_key and _extract_param for robust signature-based argument binding.
| async def wrapper(*args, **kwargs): | |
| key = _build_key(fn.__qualname__, args, kwargs, param_names, key_params) | |
| confidence = _registry.get_confidence(key) | |
| try: | |
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | |
| except (ValueError, TypeError): | |
| refresh_interval = default_refresh_interval | |
| trajectory = _extract_param(args, kwargs, param_names, 'trajectory') | |
| sampling_params = _extract_param(args, kwargs, param_names, 'sampling_params') | |
| async def wrapper(*args, **kwargs): | |
| key = _build_key(sig, fn.__qualname__, args, kwargs, key_params) | |
| confidence = _registry.get_confidence(key) | |
| try: | |
| refresh_interval = int(os.environ.get(refresh_env_var, default_refresh_interval)) | |
| except (ValueError, TypeError): | |
| refresh_interval = default_refresh_interval | |
| trajectory = _extract_param(sig, args, kwargs, 'trajectory') | |
| sampling_params = _extract_param(sig, args, kwargs, 'sampling_params') |
PR type
PR information
Write the detail information belongs to this PR.
Experiment results
Paste your experiment result here(if needed).